feat: implement statefulset primitive#25
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new statefulset workload primitive to the operator-component-framework, matching the existing primitive patterns (builder/resource, mutation planning, flavors, and lifecycle handlers), plus accompanying editor support, docs, and an end-to-end example.
Changes:
- Introduces
pkg/primitives/statefulset/with builder/resource, mutator, handlers, and flavors (plus tests). - Adds
StatefulSetSpecEditorunderpkg/mutation/editors/(plus tests) to support typed spec mutations. - Adds documentation and a runnable example demonstrating the new primitive.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/primitives/statefulset/resource.go | Implements the StatefulSet resource wrapper and default field applicator behavior. |
| pkg/primitives/statefulset/mutator.go | Adds the statefulset mutator with feature-bounded planning and apply ordering (incl. VCT ops). |
| pkg/primitives/statefulset/handlers.go | Provides default converge/grace/suspend handlers for StatefulSets. |
| pkg/primitives/statefulset/flavors.go | Adds field-application flavors for preserving labels/annotations (object + pod template). |
| pkg/primitives/statefulset/builder.go | Provides fluent builder API wiring defaults, mutations, flavors, and handlers. |
| pkg/primitives/statefulset/mutator_test.go | Tests mutator semantics (ordering, snapshots, presence ops, convenience helpers). |
| pkg/primitives/statefulset/handlers_test.go | Tests default handler behavior for converge/grace/suspension. |
| pkg/primitives/statefulset/flavors_test.go | Tests flavor ordering and preservation semantics. |
| pkg/primitives/statefulset/builder_test.go | Tests builder validation and option wiring. |
| pkg/mutation/editors/statefulsetspec.go | Introduces a typed editor for appsv1.StatefulSetSpec. |
| pkg/mutation/editors/statefulsetspec_test.go | Verifies StatefulSetSpecEditor setters and Raw() behavior. |
| examples/statefulset-primitive/main.go | Runnable example using a fake client to reconcile across spec changes and suspension. |
| examples/statefulset-primitive/resources/statefulset.go | Builds a StatefulSet resource with mutations, flavors, custom handlers, and data extraction. |
| examples/statefulset-primitive/features/mutations.go | Example feature mutations demonstrating container edits/presence and metadata edits. |
| examples/statefulset-primitive/features/flavors.go | Example custom flavors and custom converge/grace/suspend behaviors. |
| examples/statefulset-primitive/app/owner.go | Re-exports shared example CRD types for the example package. |
| examples/statefulset-primitive/app/controller.go | Example controller wiring the component framework to the statefulset primitive. |
| examples/statefulset-primitive/README.md | Documentation on running and understanding the example. |
| docs/primitives/statefulset.md | User-facing docs for the new statefulset primitive API and behavior. |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
|
approve |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
<!-- claude-review-cycle --> |
Claude Review Cycle 1 - Follow-up FixThe Fixed in commit 75657ad — <!-- claude-review-cycle-followup --> |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds builder, resource, mutator, flavors, and handlers for the StatefulSet workload primitive, following the same patterns as the Deployment primitive. Key differences from Deployment: - DefaultFieldApplicator preserves VolumeClaimTemplates (immutable after creation) - Mutator supports EnsureVolumeClaimTemplate/RemoveVolumeClaimTemplate operations - EditStatefulSetSpec provides typed access to StatefulSet-specific fields - VCT operations run after all container edits in the apply order Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates building a StatefulSet with feature mutations, flavors, custom status handlers, suspension logic, and data extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align StatefulSet mutator's EditContainers docstring with the deployment mutator by documenting that selectors target baseline/presence-added containers and should not rely on earlier edits changing match results. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 1 CompleteAddressed: Intentionally ignored:
<!-- claude-review-cycle --> |
| res, _ := NewBuilder(desired). | ||
| WithFieldApplicationFlavor(PreserveCurrentLabels). | ||
| Build() | ||
|
|
There was a problem hiding this comment.
These subtests ignore the error returned by Build(). If builder validation fails, the test will panic later (nil res) or hide the real failure. Please assert require.NoError(t, err) and only proceed when res is non-nil.
| res, _ := NewBuilder(desired). | ||
| WithFieldApplicationFlavor(flavor1). | ||
| WithFieldApplicationFlavor(flavor2). | ||
| Build() | ||
|
|
There was a problem hiding this comment.
The error returned by Build() is ignored here. Please capture it and use require.NoError(t, err) so the test fails with the correct cause if builder validation/configuration is broken.
| res, _ := NewBuilder(desired). | ||
| WithFieldApplicationFlavor(flavor). | ||
| Build() | ||
|
|
There was a problem hiding this comment.
Build()'s returned error is ignored. Please assert it with require.NoError before using res, otherwise the test may fail with a nil dereference instead of the underlying build error.
Capture and assert the error from Build() with require.NoError in all three TestMutate_OrderingAndFlavors subtests so that a builder failure surfaces as a clear test error instead of a nil-pointer panic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 2 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
| - **Custom Status Handlers**: Overriding the default logic for determining readiness (`ConvergeStatus`) and health | ||
| assessment during rollouts (`GraceStatus`). |
There was a problem hiding this comment.
The README references ConvergeStatus, but the framework/API uses ConvergingStatus (and the builder hook is WithCustomConvergeStatus). Please update the wording so readers can find the correct method/type names.
| - **Custom Status Handlers**: Overriding the default logic for determining readiness (`ConvergeStatus`) and health | |
| assessment during rollouts (`GraceStatus`). | |
| - **Custom Status Handlers**: Overriding the default logic for determining readiness (via `ConvergingStatus` and the | |
| `WithCustomConvergeStatus` builder hook) and health assessment during rollouts (`GraceStatus`). |
Update `ConvergeStatus` to `ConvergingStatus` and mention the `WithCustomConvergeStatus` builder hook so readers can find the correct method/type names in the codebase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 3 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
| ## Running the Example | ||
|
|
||
| You can run this example directly using `go run`: | ||
|
|
||
| ```bash | ||
| go run examples/statefulset-primitive/main.go | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This PR adds a new example, but the repository’s make run-examples target runs an explicit list of go run ./examples/<name>/. commands (Makefile:114-119) and currently won’t exercise statefulset-primitive. Consider updating the Makefile so the new example is covered by the standard example run target.
| } | ||
|
|
||
| res, err := NewBuilder(desired). | ||
| WithFieldApplicationFlavor(flavor). | ||
| Build() | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
These tests ignore the error returned by Build() (using res, _ := ...). If Build fails, the assertions on the error message may be misleading because the failure could be from Build rather than Mutate; assert require.NoError(t, err) on Build and then check the Mutate error.
| // StatefulSetSpecEditor provides a typed API for mutating a Kubernetes StatefulSetSpec. | ||
| type StatefulSetSpecEditor struct { | ||
| spec *appsv1.StatefulSetSpec | ||
| } | ||
|
|
||
| // NewStatefulSetSpecEditor creates a new StatefulSetSpecEditor for the given StatefulSetSpec. | ||
| func NewStatefulSetSpecEditor(spec *appsv1.StatefulSetSpec) *StatefulSetSpecEditor { | ||
| return &StatefulSetSpecEditor{spec: spec} | ||
| } |
There was a problem hiding this comment.
PR description/checklist says shared file modifications are limited to documentation updates, but this PR adds a new shared editor (StatefulSetSpecEditor) under pkg/mutation/editors (plus examples and a new primitive). Please update the PR description/checklist to reflect the actual scope so reviewers know to expect shared Go code changes.
The new statefulset-primitive example was not included in the run-examples target, so it was not exercised by `make run-examples`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 4 CompleteAddressed:
Intentionally ignored:
<!-- claude-review-cycle --> |
Align StatefulSet mutator with deployment/configmap: NewMutator no longer creates an initial feature plan. BeginFeature must be called before registering any mutations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align the statefulset primitive with the framework's move to Server-Side Apply. DefaultFieldApplicator, WithCustomFieldApplicator, WithFieldApplicationFlavor, and all flavor helpers are removed. The generic builder constructor call drops the defaultApplicator parameter. Tests now pass Object() output to Mutate() instead of empty structs. Docs and examples are updated accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| run-examples: ## Run all examples to verify they execute without error. | ||
| go run ./examples/deployment-primitive/. | ||
| go run ./examples/configmap-primitive/. | ||
| go run ./examples/statefulset-primitive/. | ||
| go run ./examples/custom-resource-implementation/. |
There was a problem hiding this comment.
The PR description/checklist states “Shared file modifications are limited to documentation updates”, but this PR also changes the Makefile (adds the new example to run-examples). Either update the PR description/checklist to reflect this, or move this change out if it’s intentionally out-of-scope.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| // EnsureVolumeClaimTemplate records that a PersistentVolumeClaim template must be | ||
| // present in the StatefulSet. If a VolumeClaimTemplate with the same name exists, | ||
| // it is replaced; otherwise, it is appended. | ||
| // | ||
| // Note: VolumeClaimTemplates are immutable once the StatefulSet exists in the cluster. | ||
| // Changes to VolumeClaimTemplates on an existing StatefulSet will be rejected by | ||
| // the Kubernetes API server. This method is primarily useful for initial creation | ||
| // or when recreating a StatefulSet. | ||
| func (m *Mutator) EnsureVolumeClaimTemplate(pvc corev1.PersistentVolumeClaim) { |
There was a problem hiding this comment.
The GoDoc for EnsureVolumeClaimTemplate says changes on an existing StatefulSet will be rejected by the API server, but this mutator currently skips all volumeClaimTemplateOps when ResourceVersion is non-empty (see Apply). That makes the behavior a silent no-op rather than an API rejection. Update the comment (and/or method docs) to explicitly state that these ops are ignored for existing resources (and when that “existing” determination applies in this framework).
Clarify that VolumeClaimTemplate operations are silently skipped for existing resources (non-empty ResourceVersion) during Apply, rather than stating they would be rejected by the API server. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
| func (m *Mutator) EditContainers(selector selectors.ContainerSelector, edit func(*editors.ContainerEditor) error) { | ||
| if selector == nil || edit == nil { | ||
| return | ||
| } | ||
| m.active.containerEdits = append(m.active.containerEdits, containerEdit{ | ||
| selector: selector, | ||
| edit: edit, | ||
| }) | ||
| } |
There was a problem hiding this comment.
All mutation registration methods dereference m.active, but m.active is only set by BeginFeature(). Calling EditContainers (and similarly EnsureContainer, EditPodSpec, etc.) before BeginFeature() will deterministically panic with a nil-pointer dereference. Consider enforcing the invariant by either: (a) calling BeginFeature() in NewMutator, (b) lazily creating a plan when m.active == nil, or (c) returning a descriptive error (requires changing method signatures) / explicit panic with a clear message.
docs/primitives.md
Outdated
| | Editor | Scope | | ||
| | ----------------------- | ----------------------------------------------------------------------- | | ||
| | `ContainerEditor` | Environment variables, arguments, resource limits, ports | | ||
| | `PodSpecEditor` | Volumes, tolerations, node selectors, service account, security context | | ||
| | `DeploymentSpecEditor` | Replicas, update strategy, label selectors | | ||
| | `StatefulSetSpecEditor` | Replicas, service name, pod management policy, update strategy | | ||
| | `ConfigMapDataEditor` | `.data` entries — set, remove, deep-merge YAML patches, raw access | | ||
| | `ObjectMetaEditor` | Labels and annotations on any Kubernetes object | |
There was a problem hiding this comment.
These tables use || as the row prefix, which in Markdown creates an empty first column and can render incorrectly (especially now that the indentation was removed). Converting these to standard Markdown table rows starting with a single | (and ensuring consistent column separators) will make the docs render as intended. The same formatting issue also appears in the 'Built-in Primitives' table further down in this file.
| | Capability | Detail | | ||
| | --------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | **Health tracking** | Verifies `ObservedGeneration` matches `Generation` before evaluating `ReadyReplicas`; reports `Healthy`, `Creating`, `Updating`, or `Scaling`; grace handler can mark Down/Degraded | | ||
| | **Rollout health** | Surfaces stalled or failing rollouts by transitioning the resource to `Degraded` or `Down` (no grace-period timing) | | ||
| | **Suspension** | Scales to zero replicas; reports `Suspending` / `Suspended` | | ||
| | **Mutation pipeline** | Typed editors for metadata, statefulset spec, pod spec, containers, and volume claim templates | |
There was a problem hiding this comment.
The capability table (and other tables in this new doc) use || prefixes, which typically renders as an extra empty column in Markdown. Switching table rows to start with a single | will avoid the empty-column artifact and improve rendering consistency across Markdown viewers.
| run-examples: ## Run all examples to verify they execute without error. | ||
| go run ./examples/deployment-primitive/. | ||
| go run ./examples/configmap-primitive/. | ||
| go run ./examples/statefulset-primitive/. |
There was a problem hiding this comment.
The PR description/checklist states that 'Shared file modifications are limited to documentation updates', but this PR also changes shared/non-doc files (e.g., Makefile and pkg/mutation/editors/statefulsetspec.go). Either update the PR description/checklist item to reflect the actual scope, or move these non-doc changes into a separate PR if that restriction is important for review/release processes.
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
|
Implements the
statefulsetKubernetes resource primitive following the pattern established by the existingConfigMapandDeploymentprimitives.Summary
statefulsetprimitive package underpkg/primitives/statefulset/docs/primitives.md) to list the new primitiveChecklist
Makefile(adding the statefulset example target), and a newStatefulSetSpecEditorinpkg/mutation/editors/